feat!: disable ibc upgrade proposal handler #2574
Conversation
cmwaters
added
the
backport:v1.x
mind elaborating on the flow that we would go through? I'm good with merging this now, and then we can expand on the existing tests to ensure that we don't accidently enable this |
|
I create a governance proposal with |
|
@evan-forbes do you want me to try write a test? |
this working makes sense to me, but it would be good practice I think to write a test to ensure that this can actually happen and we're not missing something small or hidden before creating a consensus breaking change with some potetially important (albiet temporary as we could always fix with a hardfork) consequences with IBC |
|
I'm still missing as to why this exists in the first place tbh. edit: per a sync discussion, this still isn't documented that well, but does make more sense to me now. This seems to just be a helper function to update the client state and schedule an upgrade simultaneously. Imo its simpler to just create a normal upgrade and handle everything there, including updating the client state if necessary. |
|
I've added a test to the Just as further context, the reason why there are two governance related messages for invoking a classic sdk styled upgrade is one is a normal state machine breaking change while the ibc upgrade is for ibc specific breaking changes whereby the client state needs to be upgraded. |
Codecov Report
@@ Coverage Diff @@
## main #2574 +/- ##
==========================================
- Coverage 20.64% 20.63% -0.02%
==========================================
Files 132 133 +1
Lines 15334 15344 +10
==========================================
Hits 3166 3166
- Misses 11865 11875 +10
Partials 303 303
|
| signer, err := testnode.NewSignerFromContext(s.cctx, acc) | ||
| require.NoError(t, err) | ||
| subCtx, cancel := context.WithTimeout(s.cctx.GoContext(), time.Minute) | ||
| defer cancel() |
The proposal handler to upgrade IBC also invokes the standard `ScheduleUpgrade` within the upgrading module. See https://github.com/cosmos/ibc-go/blob/e2201aaf1b016356bbd40fcdc17988437adce5ae/modules/core/02-client/keeper/proposal.go#L82. This effectively means that one can create a governance proposal to upgrade the chain which is something we didn't want to support. As there is no need to handle updating the IBC client yet, this temporarily disables the proposal type. (cherry picked from commit 03b83f8)
This is an automatic backport of pull request #2574 done by [Mergify](https://mergify.com). --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Callum Waters <[email protected]>
The proposal handler to upgrade IBC also invokes the standard
ScheduleUpgradewithin the upgrading module. See https://github.com/cosmos/ibc-go/blob/e2201aaf1b016356bbd40fcdc17988437adce5ae/modules/core/02-client/keeper/proposal.go#L82. This effectively means that one can create a governance proposal to upgrade the chain which is something we didn't want to support. As there is no need to handle updating the IBC client yet, this temporarily disables the proposal type.